@ngtools/webpack: Enable the emit of declaration files#32763
@ngtools/webpack: Enable the emit of declaration files#32763lmartorella wants to merge 3 commits intoangular:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the capability for the Angular webpack plugin to emit TypeScript declaration files, which is a valuable feature. The changes to capture the .d.ts file content from the compiler are well-implemented. However, the method used to write these files to disk in packages/ngtools/webpack/src/ivy/loader.ts has significant issues. It uses synchronous file I/O which will block the event loop and degrade performance, and it doesn't ensure the target directory exists, which could cause crashes. I've left specific comments with suggestions for improvement. Addressing these points will be crucial for making this feature production-ready.
| const relDir = path.relative(fileEmitter.compilerOptions.baseUrl, target); | ||
| target = path.join(fileEmitter.compilerOptions.declarationDir, relDir); | ||
| } | ||
| fs.writeFileSync(target, result.declaration); |
There was a problem hiding this comment.
This synchronous file write has two main problems:
- It's a blocking I/O call that will negatively impact performance, especially in watch mode.
- It will throw an error if the parent directory of
targetdoes not exist.
Ideally, this should be an asynchronous operation using fs.promises.writeFile after ensuring the directory exists with fs.promises.mkdir. This would require restructuring the promise handling in this loader to await the file I/O before calling the webpack callback.
As a minimal fix that addresses the potential crash, you could use fs.mkdirSync(path.dirname(target), { recursive: true }); before this line, but the performance issue with synchronous I/O would remain a significant concern.
There was a problem hiding this comment.
Obviously I expect a cached mechanism to avoid to emit the files immediately or before returning the async call. Please point to a possible usage of output-only caching support already part of the project, if one.
|
|
||
| // Write the declaration file in the target dir | ||
| if (result.declaration && fileEmitter.compilerOptions.declaration) { | ||
| let target = this.resourcePath.replace('.ts', '.d.ts'); |
There was a problem hiding this comment.
Using String.prototype.replace() with a string argument is not robust. It only replaces the first occurrence and doesn't handle different TypeScript file extensions like .mts or .cts correctly. A regular expression should be used to safely replace the file extension and handle all variants.
| let target = this.resourcePath.replace('.ts', '.d.ts'); | |
| let target = this.resourcePath.replace(/(\.[cm]?ts)$/, '.d$1'); |
|
Declaration emitting is disabled by design in For library builds, ng-packagr remains the only officially supported tool. Additionally, please keep in mind that the Webpack build pipeline is no longer receiving new features. |
2 participants
PR Checklist
Please check to confirm your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently the
AngularWebpackPluginis swallowing any.d.tsfile. This differs from what the standardts-loaderplugin does: it normally produces a .d.ts file for each source .ts file, alongside the source itself.If the
tsconfigdeclarationDirproperty is used, the root folder of the declaration files can be changed instead (but it keeps the source folder structure).Currently this is a viable workaround for Angular libraries:
However this produces declaration files that doesn't contains Ivy metadata, so it cannot be used by other Angular libraries.
Using the official
ng-packagrto build Angular libraries leverages a different stack (rollup), so any webpack-based solution with custom plugins cannot be easily ported tong-packagr.This PR only shows the feasibility for producing the .d.ts again (with Ivy metadata), but it is not production ready.
Is there any reasons for not considering the declaration files emission for webpack plugin?
What is the new behavior?
New behavior aligns the angular webpack plugin to
ts-loader, emitting the unbundled .d.ts files in place (or in the targetdeclarationDir) with Ivy information, at par with the ones ofng-packagr.Does this PR introduce a breaking change?
Other information